fix(container-run.sh): pin the dev/build container image by digest - #11422
Open
basvandijk wants to merge 24 commits into
Open
fix(container-run.sh): pin the dev/build container image by digest#11422basvandijk wants to merge 24 commits into
basvandijk wants to merge 24 commits into
Conversation
container-run.sh pulled ghcr.io/dfinity/{ic-dev,ic-build}:<TAG> by mutable
registry tag (TAG is only a hash of the Dockerfile inputs), so anyone with
`packages: write` on the two GHCR packages could re-point the tag and have
their image run as root on every fresh machine, with ~/.ssh, ~/.aws and the
SSH agent bind-mounted (security finding 4006141, CWE-494). Every other
consumer already pins these images by immutable @sha256 digest.
- Commit the two OCI index digests as ci/container/{ic-dev,ic-build}.digest,
next to TAG; container-autobuild.yml validates and writes them.
- container-run.sh: when the working tree's computed tag matches TAG, the
only registry fetch is `pull <repo>@<pinned digest>`; the local image must
list that digest in .RepoDigests; it is aliased to <repo>:<TAG>; `run`
gets --pull=never last. A malformed/missing pin fails before any network.
When the tag differs (Dockerfile edited, bot commit not yet pulled) the
image is built locally, never pulled by tag. If the pinned pull fails, an
existing local <repo>:<TAG> is reused with a loud warning, else built
locally. CONTAINER_RUN_REQUIRE_PINNED=1 turns every unverified path into a
hard error. The local build now passes --container-cmd so build-image.sh
builds into the store the script runs from.
- ci-main.yml test-container-run: pin-consistency check, TAG-sync gate, and
legs for a poisoned local tag, a malformed pin, an unknown digest and the
docker runtime; targets.py and the paths filters include the digest files.
- Docs: CLAUDE.md, ci/container/README.md, run-in-dev-container skill.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
On GitHub-hosted runners the runner's uid is 1001, so container-run.sh maps it to the buildifier user, who cannot read /home/ubuntu; the leg's `test -f /home/ubuntu/.ic-build-container` therefore failed although the pinned image was pulled, verified and run. The squatting image is an empty tar that cannot run anything, so `true` already proves the pinned image ran; the re-point post-check covers the rest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Pin validation misses mutable references, and manual fork PR CI is incorrectly treated as a non-PR run.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Pins dev/build container execution to immutable OCI digests.
Changes:
- Adds and validates image digest pins.
- Enforces digest-only pulls with controlled fallbacks.
- Expands CI checks, tests, filters, and documentation.
File summaries
| File | Description |
|---|---|
ci/scripts/targets.py |
Treats digest changes as global CI inputs. |
ci/container/README.md |
Documents image pinning and fallbacks. |
ci/container/ic-dev.digest |
Adds the dev image pin. |
ci/container/ic-build.digest |
Adds the build image pin. |
ci/container/container-run.sh |
Implements verified digest-based execution. |
.github/workflows/container-autobuild.yml |
Updates digest pins after builds. |
.github/workflows/ci-main.yml |
Adds pin validation and runtime tests. |
.claude/skills/run-in-dev-container/SKILL.md |
Documents pinned container behavior. |
.claude/CLAUDE.md |
Adds digest maintenance guidance. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…rite
Deliberately out-of-sync TAG (the trigger container-autobuild.yml itself
documents). The container inputs are unchanged, so the autobuild rebuilds
and re-pushes the same tag with new digests and its bot commit must restore
TAG and rewrite ci/container/{ic-dev,ic-build}.digest together with the
devcontainer and workflow pins. On this commit test-container-run skips via
the TAG-sync gate with a notice.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…-pin rewrite This reverts commit 1bafba7. The autobuild it triggered (run 33643999697) fails while building ic-build at the Dockerfile's AFL++ step (`ld: error: unable to find library -lz`), which is unrelated to this PR: the Dockerfile is unchanged and the same step fails on master's inputs. No bot commit can land until that is fixed, so restore TAG to the hash the committed digest pins were built from, keeping the branch in sync and mergeable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- The pin-consistency check now collects every ic-build reference under
.github/workflows/ and every ic-dev reference in .devcontainer/ (':tag'
or '@digest', well-formed or not) and requires the whole set to equal the
committed digest reference, so a reference changed to a mutable tag or a
malformed digest is rejected instead of being skipped by the regex.
- ci-main.yml gains an optional boolean input 'pull-request' that
ci-kickoff-manual.yml passes as true, so CI dispatched for a reviewed PR
(the only CI path for fork PRs, whose autobuild cannot commit TAG and the
digests) gets the TAG-sync notice-and-skip like a pull_request event,
while push, merge-queue and release-testing runs keep the hard error.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…_ALLOW_UNPINNED opts out) Replace the opt-in CONTAINER_RUN_REQUIRE_PINNED with the inverse default: container-run.sh now refuses to run any image that is not verified against the committed digest, and CONTAINER_RUN_ALLOW_UNPINNED=1 opts out for one run (build locally when the working tree has no reviewed pin, or reuse an existing local image when the pinned pull fails), announced with a warning. The users exposed by the original finding (fresh machines, external repro-check verifiers, the cloud-config VM, stale checkouts) are exactly those who never set an opt-in knob, and a failed pinned pull is better as a fast, explicit error than as a surprise 30-minute local build. The cost is one env var for developers editing the Dockerfile, who faced a local build anyway. CI needs no env any more; the refusal legs now test the default. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The opt-out accepts unintended values and the consistency gate misses some mutable image-reference locations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/ci-main.yml:885
- Docker is a supported runtime whose image lookup, digest verification, tagging, and
--pull=neverpaths are changed here, butcontinue-on-errormakes its only CI leg unable to catch regressions. Once the leg has demonstrated green on this PR, make it blocking before merge.
.github/workflows/ci-main.yml:1006 - The consistency gate only searches for
ic-devin the devcontainer andic-buildin workflows. A mutableic-dev:tagadded to a workflow—oric-build:tagadded to the devcontainer—therefore passes this check, leaving the pinning invariant unenforced. Search both consumer locations for each image.
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
PRs from forks cannot change the container inputs: touching ci/* closes them automatically (.github/repo_policies/EXTERNAL_CONTRIB_BLACKLIST), so CI dispatched for a fork PR by ci-kickoff-manual.yml can never see an out-of-sync ci/container/TAG. The TAG-sync gate therefore keys on the pull_request event alone again; a manual dispatch with an out-of-sync TAG is a hard error like every other non-PR run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The opt-out currently accepts any nonempty value, and the autobuild guidance contradicts the fail-closed behavior.
Review details
Suppressed comments (2)
ci/container/container-run.sh:134
- Every nonempty value enables the security opt-out because all downstream checks use
-n/-z. Thus conventional settings such asCONTAINER_RUN_ALLOW_UNPINNED=0orfalsesilently permit a cached or locally built unverified image, contrary to the documented=1and fail-closed behavior. Only the exact value1should enable this path.
ALLOW_UNPINNED="${CONTAINER_RUN_ALLOW_UNPINNED:-}"
.github/workflows/container-autobuild.yml:49
- This workflow hint says
container-run.shbuilds locally whileTAGis out of sync, but the new default behavior is to refuse unless the explicit opt-out is set. The current message will send users to a command that still fails; mention the required environment variable.
echo "Modify ci/container/TAG with a random string to trigger a new build; until this"
echo "workflow's commit lands, container-run.sh on that commit builds the image locally."
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
It has passed on every run of this PR, so the continue-on-error hedge only hides regressions in the docker-specific code paths now. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The opt-out accepts unintended values, and the consistency gate does not inspect both image types across every covered consumer location.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/ci-main.yml:990
- This only scans
.devcontainer, so a workflow can add a mutableghcr.io/dfinity/ic-dev:<tag>reference and still pass the pin-consistency gate. Scan both consumer locations to enforce the stated invariant for everyic-devreference.
This issue also appears on line 996 of the same file.
ci/container/container-run.sh:134
- The opt-out is enabled by any non-empty value, so
CONTAINER_RUN_ALLOW_UNPINNED=0,false, or a typo silently permits an unverified privileged image even though the documented security contract requires exactly=1. Reject values other than1so accidental environment configuration remains fail-closed.
ALLOW_UNPINNED="${CONTAINER_RUN_ALLOW_UNPINNED:-}"
.github/workflows/ci-main.yml:996
- This scans workflows only, so a mutable
ic-build:<tag>introduced indevcontainer.jsonis not checked even though changes there trigger this gate. Include the devcontainer file as well so both image names are pinned across all covered consumers.
build_refs="$(grep -rhoE "${prefix}ic-build[:@][^\"'[:space:]]+" .github/workflows/ | sort -u)"
.github/workflows/container-autobuild.yml:49
- This hint says
container-run.shbuilds automatically whileTAGis out of sync, but the new default path refuses to run; a local build only occurs withCONTAINER_RUN_ALLOW_UNPINNED=1. Update the hint so users do not interpret the expected refusal as a failure of the workflow.
echo "Modify ci/container/TAG with a random string to trigger a new build; until this"
echo "workflow's commit lands, container-run.sh on that commit builds the image locally."
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
- container-run.sh accepts only the exact value 1 for CONTAINER_RUN_ALLOW_UNPINNED and refuses any other non-empty value, so 0, false or a typo cannot silently disable digest enforcement. - The pin-consistency check scans both consumer locations (.devcontainer/ and .github/workflows/) for both image names, so a mutable ic-dev reference in a workflow or an ic-build reference in devcontainer.json is rejected too. - The autobuild's forced-rebuild hint now says container-run.sh refuses on the out-of-sync commit and names the opt-out for a local build. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Tagging failures are silently ignored, and the no-reference CI diagnostic exits prematurely.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
ci/container/container-run.sh:209
- Do not silently ignore a failed alias operation. If this command fails, a squatting tag remains in place and the verified digest-only image can be removed by
docker image prune, even though the script reports success and the PR relies on the tag being re-pointed. Let the failure abort so this postcondition is guaranteed.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
- The pin-consistency check treats grep's no-match exit as an empty set so that, under pipefail, the '<none>' diagnostic and ::error annotation are emitted instead of the step aborting silently. - container-run.sh no longer ignores a failed 'tag' of the verified image: the alias re-points a squatting tag and keeps the image out of 'docker image prune', and the CI post-check relies on it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The cached mutable-tag fallback after a failed digest pull lacks regression coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/ci-main.yml:919
- This test starts with an empty image store, so it does not cover the critical fail-closed branch where the pinned pull fails while a mutable-tag image is already cached. The separate poisoned-tag case uses a valid digest, so both tests would still pass if a future change reused the cached tag after a failed pull. Add a case that combines the poisoned local tag with the unknown digest and asserts that
container-run.shrefuses rather than running it.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Copilot pointed out that the refusal legs of test-container-run all start from an empty image store, so none of them covered the case where the pinned pull fails while an image is already cached under the mutable tag. The poisoned-tag leg uses the real pin, so a regression that fell back to the cached tag after a failed pull would have passed both. Add a leg that combines the squatting local tag with an unknown digest and expects the same refusal. The fast-refusal assertion now snapshots the image store and container list before the run and requires both to be unchanged afterwards, so it also catches a cached image being run, instead of only checking that no ic-dev image exists. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive privileged-container and CI workflow changes merit final human validation despite no concrete defects found.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Use bash's redirection form instead of forking cat, the idiom container-run.sh already uses for the same files. The steps run under the workflow's default shell, which is bash on the Linux runners. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Assign $1 and $2 to local image and digest variables instead of relying on a comment on the function's opening line; $1 alone appeared five times, including in the long error message. No behavioural change, and the image name still never appears as a literal registry reference, so container-autobuild.yml's sed rewrite of the workflow is unaffected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The security-critical out-of-sync execution branch lacks automated regression coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
ci/container/container-run.sh:179
- This out-of-sync branch is never exercised by the new CI matrix: the preceding TAG-sync gate skips every run when the checkout is out of sync. That leaves the fail-closed behavior for edited container inputs—one of the security-critical paths added here—without regression coverage. Add a leg whose setup modifies a hashed input after the sync check and assert that the script refuses by default (and ideally a cached-local-image leg for the explicit opt-out).
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Copilot pointed out that the out-of-sync branch, where the container inputs differ from what ci/container/TAG was built from, was never exercised: the TAG-sync gate skips every leg on an out-of-sync checkout. Add two legs whose setup step edits ci/container/Dockerfile after the gate has passed. By default the script must refuse without pulling or building anything. With CONTAINER_RUN_ALLOW_UNPINNED=1 it must reuse an image cached under the computed tag (seeded by re-tagging the pinned image) and warn that the image is unverified. To support that, the matrix gains optional allow_unpinned and expect_warning keys, and the success path captures the output so the warning can be asserted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The expanded test matrix allocates nine runners for every CI invocation, including unrelated pull requests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot pointed out that the matrix job had no job-level condition, so every ci-main run allocated one runner per leg (nine after the recent additions) only to skip the steps via the path filter. Move the checkout, path filter, decision, pin-consistency check and TAG-sync gate into test-container-run-preflight, a single-runner job whose `run` output is the sync result. The matrix job needs it and only exists when that output is 'true', so unrelated PRs allocate one short runner instead of nine, and the pin and TAG checks run once instead of once per leg. The per-step in_sync conditions are gone for the same reason. The legs themselves are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The security-critical privileged-container path is well covered, but the autobuild digest rewrite could not be exercised end-to-end.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
basvandijk
changed the title
basvandijk
marked this pull request as ready for review
September 3, 2026 12:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fix a Claude Mythos security finding by forcing
ci/container/container-run.shto pull its image pinned by a digest instead of using a mutable tag.Why
ci/container/container-run.shis run by both developers and external repro-check verifiers. To reduce the risk of it pulling in a compromised image it should pull in images pinned by a digest instead of a tag that could be locally mutated.This also makes it consistent with all other sites that pull images which all pull by digest:
.devcontainer/devcontainer.jsonand.github/workflows/*.Future work could be to further extend this with only running signed / attested images.
How
container-autobuild.ymlnow writes the image digests toci/container/ic-dev.digestandci/container/ic-build.digest.ci/container/container-run.shwill now read the digest file and check if a local image with that digest exists.If not it will pull it by that digest.
If the pull fails or if the container input files have been modified the script will fail unless
CONTAINER_RUN_ALLOW_UNPINNEDis set to1in which case the image is build locally.The
test-container-runtest-suite inci-mainhas been extended with more test cases that check for the various edge conditions.